[WIP] CNTRLPLANE-2711: add vault kms plug configuration api#2805
[WIP] CNTRLPLANE-2711: add vault kms plug configuration api#2805flavianmissi wants to merge 4 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@flavianmissi: This pull request references CNTRLPLANE-2711 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @flavianmissi! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved the KMSEncryptionProvider feature gate and its test YAML and feature-gate registration entries. Replaced AWS-specific KMS configuration with a Vault-based provider: added 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
from
0e6ccdb to
9204eb8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 67-73: Add explicit Kubernetes validation markers to enforce the
documented bounds: annotate VaultNamespace, TransitMount, and TransitKey with
kubebuilder validation comments before each field (e.g. //
+kubebuilder:validation:MinLength=1 and //
+kubebuilder:validation:MaxLength=256) and keep the existing json tag
(json:"...,omitempty"). This ensures the CRD rejects empty strings and overly
long values for the VaultNamespace, TransitMount, and TransitKey fields.
- Around line 57-65: The VaultAddress field currently allows both http and
https; update its validation to require HTTPS only by changing the kubebuilder
XValidation rule on VaultAddress to match '^https://', update the validation
message to say "vaultAddress must be a valid URL starting with 'https://'...",
and ensure the example and any documentation reference use https (e.g.,
'https://vault.example.com:8200'); modify the comment and the
+kubebuilder:validation:XValidation line for VaultAddress in
types_kmsencryption.go accordingly.
- Around line 5-24: The change removed AWS KMS support and will break upgrades
for clusters with persisted AWS configs; restore backward compatibility by
either reintroducing AWS as a deprecated union member on KMSConfig (add
AWSKMSProvider value to KMSProviderType and an AWSKMSConfig struct as a
+unionMember with deprecation tags) and implement conversion logic in the API
conversion/webhook handlers to migrate existing AWSKMSConfig to the new model,
or implement and wire a documented controller migration path that detects
existing KMSConfig entries with AWS data and converts them to the new Vault-only
shape (or marks them as exempt) during upgrade; touch KMSConfig,
KMSProviderType, VaultKMSConfig, and add AWSKMSConfig/AWSKMSProvider identifiers
and ensure feature-gate annotations and validation rules account for
preserved/deprecated AWS entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 31c84276-1bd6-44d1-aef1-c88652b3bb69
⛔ Files ignored due to path filters (10)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (17)
config/v1/tests/apiservers.config.openshift.io/KMSEncryption.yamlconfig/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/types_apiserver.goconfig/v1/types_kmsencryption.gofeatures.mdfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
💤 Files with no reviewable changes (12)
- config/v1/tests/apiservers.config.openshift.io/KMSEncryption.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- features/features.go
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- features.md
- config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
config/v1/types_kmsencryption.go (2)
67-73:⚠️ Potential issue | 🟠 MajorAdd the length validators promised by the API contract.
The documentation states bounds for
vaultNamespace(1-256),transitMount(1-128), andtransitKey(1-128), but the kubebuilder markers are missing. This allows empty strings and arbitrarily long values to pass schema validation, causing runtime failures instead of admission-time rejection.Suggested fix for vaultNamespace (lines 67-73)
// vaultNamespace specifies the Vault namespace where the Transit secrets engine is mounted. // This is only applicable for Vault Enterprise installations. // The value can be between 1 and 256 characters. // When this field is not set, no namespace is used. // + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=256 // +optional VaultNamespace string `json:"vaultNamespace,omitempty"`Suggested fix for transitMount (lines 128-134)
// transitMount specifies the mount path of the Vault Transit engine. // The value can be between 1 and 128 characters. // When this field is not set, it defaults to "transit". // + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=128 // +kubebuilder:default="transit" // +optional TransitMount string `json:"transitMount,omitempty"`Suggested fix for transitKey (lines 136-141)
// transitKey specifies the name of the encryption key in Vault's Transit engine. // This key is used to encrypt and decrypt data. // The value must be between 1 and 128 characters. // + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=128 // +required TransitKey string `json:"transitKey,omitempty"`Also applies to: 128-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_kmsencryption.go` around lines 67 - 73, Add kubebuilder validation markers to enforce the documented length bounds on the three Vault fields: annotate VaultNamespace with +kubebuilder:validation:MinLength=1 and +kubebuilder:validation:MaxLength=256, annotate TransitMount with +kubebuilder:validation:MinLength=1 and +kubebuilder:validation:MaxLength=128, and annotate TransitKey with +kubebuilder:validation:MinLength=1 and +kubebuilder:validation:MaxLength=128 so the API server rejects empty or overly long values at admission time; ensure the markers appear immediately above the corresponding struct fields VaultNamespace, TransitMount, and TransitKey.
57-65:⚠️ Potential issue | 🟠 MajorRequire HTTPS for the Vault endpoint.
Allowing
http://enables cleartext transmission of AppRole credentials and KMS traffic. ThetlsVerify: SkipVerifyoption already covers insecure testing scenarios without disabling transport encryption entirely.Suggested fix
- // vaultAddress specifies the address of the HashiCorp Vault instance. - // The value must be a valid URL with scheme (http:// or https://) and can be up to 512 characters. + // vaultAddress specifies the address of the HashiCorp Vault instance. + // The value must be a valid HTTPS URL and can be up to 512 characters. // Example: https://vault.example.com:8200 // - // +kubebuilder:validation:XValidation:rule="self.matches(r'^https?://')",message="vaultAddress must be a valid URL starting with 'http://' or 'https://' (e.g., 'https://vault.example.com:8200')." + // +kubebuilder:validation:XValidation:rule="self.matches(r'^https://')",message="vaultAddress must be a valid HTTPS URL (e.g., 'https://vault.example.com:8200')." // +kubebuilder:validation:MaxLength=512 // +kubebuilder:validation:MinLength=1 // +required VaultAddress string `json:"vaultAddress,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_kmsencryption.go` around lines 57 - 65, The VaultAddress field currently allows both http and https; change its validation to require HTTPS only by updating the kubebuilder XValidation rule for VaultAddress (symbol: VaultAddress) to match r'^https://', adjust the validation message to state "vaultAddress must be a valid HTTPS URL starting with 'https://'" and keep the MaxLength/MinLength/required tags intact so the API rejects non-HTTPS Vault endpoints.
🧹 Nitpick comments (2)
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
342-346: Validation message references KMSEncryption feature gate but rule doesn't enforce it.The message says "kms config is required when encryption type is KMS and KMSEncryption feature gate is enabled" but the CEL rule itself doesn't check the feature gate state—it just validates
kmspresence based ontype == 'KMS'. This is likely correct behavior (the feature gate controls schema presence, not this runtime rule), but the message is potentially misleading.Consider simplifying the message to match the actual rule behavior:
-- message: kms config is required when encryption type is KMS and - KMSEncryption feature gate is enabled, and forbidden otherwise +- message: kms config is required when encryption type is KMS, and forbidden otherwise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml` around lines 342 - 346, The validation message is misleading because it mentions the KMSEncryption feature gate while the CEL rule only checks self.type and self.kms; update the x-kubernetes-validations.message to reflect the actual rule (e.g., "kms config is required when encryption type is 'KMS' and forbidden otherwise") so it matches the rule for x-kubernetes-validations.rule that evaluates has(self.type) && self.type == 'KMS' ? has(self.kms) : !has(self.kms); adjust only the message string associated with that rule.payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
342-346: Same validation message inconsistency as CustomNoUpgrade CRD.The message mentions "KMSEncryption feature gate is enabled" but the CEL rule doesn't actually check feature gate state. Consider the same simplification suggested for the CustomNoUpgrade variant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml` around lines 342 - 346, The validation message for the x-kubernetes-validations entry in the apiservers-TechPreviewNoUpgrade CRD is inconsistent with the CEL rule: the message references "KMSEncryption feature gate is enabled" but the rule only checks self.type and self.kms. Update the message to match the rule (e.g., "kms config is required when encryption type is KMS and forbidden otherwise") or, if you intend to gate on a feature flag, modify the CEL rule to include that feature check; edit the x-kubernetes-validations -> message and/or the rule that uses has(self.type) && self.type == 'KMS' ? has(self.kms) : !has(self.kms) so they are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 48-55: The validation regex for VaultKMSPluginImage rejects
repository names containing underscores; update the kubebuilder XValidation rule
on VaultKMSPluginImage to allow underscores in the repository path by changing
the character class for the path portion from [a-zA-Z0-9./-]+ to include
underscores (e.g., [a-zA-Z0-9._/-]+) so OCI-compliant names like
"registry.example.com/my_project/vault_plugin@sha256:..." pass validation while
preserving the rest of the pattern and MinLength/MaxLength constraints.
---
Duplicate comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 67-73: Add kubebuilder validation markers to enforce the
documented length bounds on the three Vault fields: annotate VaultNamespace with
+kubebuilder:validation:MinLength=1 and +kubebuilder:validation:MaxLength=256,
annotate TransitMount with +kubebuilder:validation:MinLength=1 and
+kubebuilder:validation:MaxLength=128, and annotate TransitKey with
+kubebuilder:validation:MinLength=1 and +kubebuilder:validation:MaxLength=128 so
the API server rejects empty or overly long values at admission time; ensure the
markers appear immediately above the corresponding struct fields VaultNamespace,
TransitMount, and TransitKey.
- Around line 57-65: The VaultAddress field currently allows both http and
https; change its validation to require HTTPS only by updating the kubebuilder
XValidation rule for VaultAddress (symbol: VaultAddress) to match r'^https://',
adjust the validation message to state "vaultAddress must be a valid HTTPS URL
starting with 'https://'" and keep the MaxLength/MinLength/required tags intact
so the API rejects non-HTTPS Vault endpoints.
---
Nitpick comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml`:
- Around line 342-346: The validation message is misleading because it mentions
the KMSEncryption feature gate while the CEL rule only checks self.type and
self.kms; update the x-kubernetes-validations.message to reflect the actual rule
(e.g., "kms config is required when encryption type is 'KMS' and forbidden
otherwise") so it matches the rule for x-kubernetes-validations.rule that
evaluates has(self.type) && self.type == 'KMS' ? has(self.kms) : !has(self.kms);
adjust only the message string associated with that rule.
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml`:
- Around line 342-346: The validation message for the x-kubernetes-validations
entry in the apiservers-TechPreviewNoUpgrade CRD is inconsistent with the CEL
rule: the message references "KMSEncryption feature gate is enabled" but the
rule only checks self.type and self.kms. Update the message to match the rule
(e.g., "kms config is required when encryption type is KMS and forbidden
otherwise") or, if you intend to gate on a feature flag, modify the CEL rule to
include that feature check; edit the x-kubernetes-validations -> message and/or
the rule that uses has(self.type) && self.type == 'KMS' ? has(self.kms) :
!has(self.kms) so they are consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b8d09f5-f990-4d5b-8bf2-27856efe1d7c
⛔ Files ignored due to path filters (8)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (12)
config/v1/types_kmsencryption.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
💤 Files with no reviewable changes (8)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
✅ Files skipped from review due to trivial changes (1)
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
from
9204eb8 to
adcbfe4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
config/v1/types_kmsencryption.go (1)
5-24:⚠️ Potential issue | 🔴 CriticalPreserve an upgrade path for existing
awsKMS configs.This change removes
awsfrom the publickmsunion and provider enum without a deprecated member or any migration path in the API surface. Clusters that already persistedspec.encryption.kms.awsunder the previous gate will be rejected on upgrade once this validation becomes authoritative.Also applies to: 27-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_kmsencryption.go` around lines 5 - 24, The KMS union removed the previously allowed "aws" provider with no migration or deprecated field, which will reject clusters that already have spec.encryption.kms.aws; restore backward compatibility by reintroducing a deprecated/hidden union member for the AWS provider (e.g. add an Aws *AWSKMSConfig `json:"aws,omitempty"` with appropriate +optional and +deprecated markers or a FeatureGate-aware validation exception) and keep the union discriminator Type KMSProviderType accepting the old "aws" enum value (or add a deprecated enum entry) so existing persisted KMSConfig objects with Type == "AWS" and the Aws member continue to validate during upgrade while new API users are prevented from creating new AWS configs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 37-79: The CRD manifests are out of sync with the VaultKMSConfig
Go type: the Go struct now uses kmsPluginImage and a nested TLS struct
(VaultTLSConfig with caBundle and serverName) but the published CRDs still
expose vaultKMSPluginImage, flat tlsCA/tlsServerName/tlsVerify and old
http:///length validations; update the CRD schema to match the Go API by
renaming fields (vaultKMSPluginImage -> kmsPluginImage), nesting TLS under tls
with caBundle and serverName fields, removing the old http:// rule and
correcting MinLength/MaxLength rules to match KMSPluginImage and VaultAddress,
then regenerate the CRD manifests (e.g., run your controller-gen/CRD generation
or project make target) so the published YAML matches VaultKMSConfig,
KMSPluginImage, VaultAddress and TLS/VaultTLSConfig definitions.
---
Duplicate comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 5-24: The KMS union removed the previously allowed "aws" provider
with no migration or deprecated field, which will reject clusters that already
have spec.encryption.kms.aws; restore backward compatibility by reintroducing a
deprecated/hidden union member for the AWS provider (e.g. add an Aws
*AWSKMSConfig `json:"aws,omitempty"` with appropriate +optional and +deprecated
markers or a FeatureGate-aware validation exception) and keep the union
discriminator Type KMSProviderType accepting the old "aws" enum value (or add a
deprecated enum entry) so existing persisted KMSConfig objects with Type ==
"AWS" and the Aws member continue to validate during upgrade while new API users
are prevented from creating new AWS configs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6481d801-9501-491c-91d0-022980739a42
⛔ Files ignored due to path filters (8)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (12)
config/v1/types_kmsencryption.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
💤 Files with no reviewable changes (8)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
2 times, most recently
from
42d4c42 to
bd0b2a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 56-60: The XValidation on vaultAddress (self.matches('^https://'))
only enforces the prefix and allows invalid values like "https://"; update the
validation to either enforce a full URL shape (replace the XValidation rule with
a stronger regex/pattern that requires a host and optional port/path, e.g.
require at least "https://<host>" semantics) or, if you prefer the lighter
check, change the documentation/message to state "must start with 'https://'" to
match the current rule; update the kubebuilder:XValidation rule and the
vaultAddress comment/message accordingly so they are consistent (referencing the
vaultAddress field and its XValidation rule).
- Line 50: The XValidation message still references the old field name
vaultKMSPluginImage; update the validation annotation on the kmsPluginImage
field so the message uses the new field name (kmsPluginImage) and keep the rest
of the guidance unchanged. Locate the kubebuilder XValidation tag shown and
replace "vaultKMSPluginImage" in the message with "kmsPluginImage" so admission
errors point to the correct field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 55d2598e-91dd-493c-9853-e17140c344fb
⛔ Files ignored due to path filters (8)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (12)
config/v1/types_kmsencryption.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
💤 Files with no reviewable changes (8)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
from
bd0b2a0 to
4e09913
Compare
|
@flavianmissi: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| // APIServerEncryption is used to encrypt sensitive resources on the cluster. | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=KMSEncryptionProvider,rule="has(self.type) && self.type == 'KMS' ? has(self.kms) : !has(self.kms)",message="kms config is required when encryption type is KMS, and forbidden otherwise" | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=KMSEncryption,rule="has(self.type) && self.type == 'KMS' ? has(self.kms) : !has(self.kms)",message="kms config is required when encryption type is KMS and KMSEncryption feature gate is enabled, and forbidden otherwise" |
There was a problem hiding this comment.
We don't need to include and KMSEncryption feature gate is enabled in the message - this validation will only be present when the KMSEncryption feature gate is enabled.
| // that will be used with KMSEncryptionProvider encryption | ||
| // +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'AWS' ? has(self.aws) : !has(self.aws)",message="aws config is required when kms provider type is AWS, and forbidden otherwise" | ||
| // that will be used with KMS encryption | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=KMSEncryption,rule="has(self.type) && self.type == 'Vault' ? (has(self.vault) && self.vault.vaultAddress != \"\") : !has(self.vault)",message="vault config is required when kms provider type is Vault, and forbidden otherwise" |
There was a problem hiding this comment.
A couple things here:
- This validation doesn't need to be feature gated because the type itself is only referenced in an already feature gated field
- This expression can be
self.type == 'Vault' ? has(self.vault) : !has(self.vault)to simplify the constraint to ensuring that thevaultfield is required when type isVaultand forbidden otherwise. You don't need thehas(self.type)because thetypefield is already required which means it will fail validation if it is omitted - which I believe is a short-circuiting validation. I assume theself.vault.vaultAddress != ""is meant to ensure the vault address is not empty, which should be enforced by making that child field required.
| // Valid values are: | ||
| // - "Vault": HashiCorp Vault KMS (available when KMSEncryption feature gate is enabled) |
There was a problem hiding this comment.
Nit: In general, we recommend a format like:
type defines the kind of platform for the KMS provider.
Allowed values are Vault.
When set to Vault, {description}.
This is because list notation can sometimes render weirdly in various contexts that display the documentation generated from the GoDoc. Using plain English sentences avoids that problem altogether.
| // +openshift:enable:FeatureGate=KMSEncryption | ||
| // +unionMember | ||
| // +optional | ||
| AWS *AWSKMSConfig `json:"aws,omitempty"` |
There was a problem hiding this comment.
I think you will have to tombstone this field instead of removing it entirely. This is to prevent us from ever re-using the serialized field name of aws because older clients that previously read this API with this field would still attempt to unmarshal the field in the same way but the underlying API shape may be different causing read issues.
Do you intend to support an AWS KMS provider in the near-ish future?
There was a problem hiding this comment.
Do you intend to support an AWS KMS provider in the near-ish future?
We'll probably need to support AWS in the future, since it's already officially supported by Hypershift.
This is to prevent us from ever re-using the serialized field
Others can correct me if I'm wrong, but we never had any clients using this field. IIUC this API type was prematurely added without any consumers. Removing this would make things cleaner and allow us to make better design decisions in the future.
I think it's safe to remove it, but I'd like to hear form @ardaguclu as well.
There was a problem hiding this comment.
Others can correct me if I'm wrong, but we never had any clients using this field. IIUC this API type was prematurely added without any consumers. Removing this would make things cleaner and allow us to make better design decisions in the future.
I think it's safe to remove it, but I'd like to hear form @ardaguclu as well.
I was asked to tombstone as well #2622 (comment). I think it might make sense tombstoning it.
We'll probably need to support AWS in the future, since Hypershift supports it already.
We probably won't support AWS in the foreseeable future.
There was a problem hiding this comment.
As Fabio said, there are no clients using this.
While we have no plans to support AWS, I think it's quite safe to assume that customers will eventually ask this from us.
Would tombstoning the field mean that we would not be able to add it back if needed?
| // AWSKMSConfig defines the KMS config specific to AWS KMS provider | ||
| type AWSKMSConfig struct { |
There was a problem hiding this comment.
If we tombstone the field, we will also need to tombstone this type.
| // | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +default="transit" |
There was a problem hiding this comment.
For configuration APIs, we normally try to avoid setting default values in this way.
Instead, we make omission of the field mean "no opinion" and have a standard terminology we use like
api/config/v1/types_ingress.go
Lines 126 to 127 in 34e6087
This allows us to modify the underlying default behavior as needed without being contractually tied to a default value.
| ApproleSecret *SecretNameReference `json:"approleSecret,omitempty"` | ||
|
|
||
| // transitMount specifies the mount path of the Vault Transit engine. | ||
| // The value can be between 1 and 1024 characters. |
There was a problem hiding this comment.
Are there any other reasonable constraints we can enforce related to formatting here?
|
|
||
| // transitKey specifies the name of the encryption key in Vault's Transit engine. | ||
| // This key is used to encrypt and decrypt data. | ||
| // The value must be between 1 and 512 characters. |
There was a problem hiding this comment.
Are there any additional constraints that we can enforce here related to formatting of the input?
| // -----END CERTIFICATE----- | ||
| // | ||
| // +optional | ||
| CABundle *ConfigMapNameReference `json:"caBundle,omitempty"` |
There was a problem hiding this comment.
Similar to my comment on SecretNameReference usage, I'd recommend against re-using this type because it does not have any validations on the input.
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:MinLength=1 |
There was a problem hiding this comment.
Any other formatting constraints we can enforce here? Presumably this needs to be a valid hostname?
5 participants
No description provided.